Skip to content
This repository has been archived by the owner on Feb 11, 2020. It is now read-only.

new peer-book #30

Merged
merged 2 commits into from
Mar 31, 2017
Merged

new peer-book #30

merged 2 commits into from
Mar 31, 2017

Conversation

daviddias
Copy link
Member

No description provided.

src/index.js Outdated
if (this._peers[peerInfo.id.toB58String()] && !replace) {
// peerInfo.replace merges by default if none to replace are passed
this._peers[peerInfo.id.toB58String()]
.multiaddrs.replace([], peerInfo.multiaddrs.toArray())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not do what we want, as I said before what I need is to merge both sets of multiaddrs, which this does not do in my tests

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/index.js Outdated
// peerInfo.replace merges by default
peers[peerInfo.id.toB58String()].multiaddr.replace([], peerInfo.multiaddrs)
/**
* Stores a peerInfo, if already exist, only adds the multiaddrs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also need public and private keys to be added if I put an existing one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more thing, it would be great if it could return the merged result, as I currently have to do this

// got info from the network
peerBook.put(info)
const correctInfo = peerBook.get(info)

// after
const correctInfo = peerBook.put(info)

@dignifiedquire
Copy link
Member

dignifiedquire commented Mar 30, 2017

  • .has(id) would also be nice.
  • something like getOrCreate I am doing quite often this at the moment
let info
try {
  info = peerBook.get(id)
}  catch(err) {
  info = new PeerInfo(id)
}

src/index.js Outdated
@@ -29,30 +34,31 @@ function PeerBook () {
* @param {PeerId} id
* @returns {PeerInfo}
*/
this.get = (id) => {
get (id) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this could accept PeerInfos as well, that would be nice

@daviddias
Copy link
Member Author

daviddias commented Mar 30, 2017

  • one another thing is that the state of the connected multiaddr also needs to be coalesced
  • always favour the instance inside peerbook, avoid replacing at all cost (to avoid funky scenarios)

@daviddias
Copy link
Member Author

daviddias commented Mar 31, 2017

@dignifiedquire did more changes than requested, but I think you will be pleased :) (still checking all the tests, because you know.. npm installs)

src/index.js Outdated
*
* @param {PeerInfo} peerInfo
* @param {replace} boolean
* @returns {null}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would love to see the one put into the book to be returned here

@daviddias daviddias merged commit fe6f44e into master Mar 31, 2017
@daviddias daviddias deleted the feat/advanced-peer-book branch March 31, 2017 10:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants